Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882

aaronburtle · 2025-09-18T01:39:47Z

Why make this change?

Adds AKV variable replacement and expands our design for doing variable replacements to be more extensible when new variable replacement logic is added.

Closes #2708
Closes #2748
Related to #2863

What is this change?

Change the way that variable replacement is handled to instead of simply using a bool to indicate that we want env variable replacement, we add a class which holds all of the replacement settings. This will hold whether or not we will do replacement for each kind of variable that we will handle replacement for during deserialization. We also include the replacement failure mode, and put the logic for handling the replacements into a strategy dictionary which pairs the replacement variable type with the strategy for doing that replacement.

Because Azure Key Vault secret replacement requires having the retry and connection settings in order to do the AKV replacement, we must do a first pass where we only do non-AKV replacement and get the required settings so that if AKV replacement is used we have the required settings to do that replacement.

We also have to keep in mind that the legacy of the Configuration Controller will ignore all variable replacement, so we construct the replacement settings for this code path to not use any variable replacement at all.

How was this tested?

We have updated the logic for the tests to use the new system, however manual testing using an actual AKV is still required.

Sample Request(s)

Example REST and/or GraphQL request to demonstrate modifications
Example of CLI usage to demonstrate modifications

src/Config/Converters/AKVRetryPolicyOptionsConverterFactory.cs

src/Config/Converters/EntityCacheOptionsConverterFactory.cs

src/Config/Converters/StringJsonConverterFactory.cs

RubenCerna2079 · 2025-10-07T00:05:30Z

src/Config/FileSystemRuntimeConfigLoader.cs


-            if (!string.IsNullOrEmpty(json) && TryParseConfig(json, out RuntimeConfig, connectionString: _connectionString, replaceEnvVar: replaceEnvVar))
+            if (!string.IsNullOrEmpty(json) && TryParseConfig(json, out RuntimeConfig, 
+                new DeserializationVariableReplacementSettings(azureKeyVaultOptions: null, doReplaceEnvVar: true, doReplaceAKVVar: true), logger: null, connectionString: _connectionString))


We also need to delete the parameter replaceEnvVar from the TryLoadConfig function since we don't use it anymore

src/Config/RuntimeConfigLoader.cs

RubenCerna2079

Still need to get rid of envVar in some places and resolve missing logic inside of an if statement.

src/Config/Converters/AzureKeyVaultOptionsConverterFactory.cs

src/Config/Converters/StringJsonConverterFactory.cs

src/Config/Converters/Utf8JsonReaderExtensions.cs

src/Config/RuntimeConfigLoader.cs

RubenCerna2079 · 2025-10-07T22:47:44Z

src/Config/DeserializationVariableReplacementSettings.cs

+                clientOptions.Retry.MaxRetries = options.RetryPolicy.MaxCount ?? 3;
+                clientOptions.Retry.Delay = TimeSpan.FromSeconds(options.RetryPolicy.DelaySeconds ?? 1);
+                clientOptions.Retry.MaxDelay = TimeSpan.FromSeconds(options.RetryPolicy.MaxDelaySeconds ?? 16);
+                clientOptions.Retry.NetworkTimeout = TimeSpan.FromSeconds(options.RetryPolicy.NetworkTimeoutSeconds ?? 30);


Shouldn't we use the DEFAULT variables found in AKVRetryPolicyOptions instead of using numbers?

RubenCerna2079 · 2025-10-07T23:13:19Z

src/Config/RuntimeConfigLoader.cs

+        catch
+        {
+            // If we can't extract AKV options, return null and proceed without AKV variable replacement
+            return null;
+        }
+
+        return null;


nit: Returning the nulls like this seems a bit redundant, do you think there is a better way to do this?

RubenCerna2079 · 2025-10-07T23:17:25Z

src/Config/DeserializationVariableReplacementSettings.cs

+    public class DeserializationVariableReplacementSettings
+    {
+        public bool DoReplaceEnvVar { get; set; } = true;
+        public bool DoReplaceAKVVar { get; set; } = true;


Suggested change

public bool DoReplaceAKVVar { get; set; } = true;

public bool DoReplaceAkvVar { get; set; } = true;

RubenCerna2079 · 2025-10-07T23:18:33Z

src/Config/RuntimeConfigLoader.cs

+            envFailureMode: replacementFailureMode);
+        options.Converters.Add(new StringJsonConverterFactory(envOnlySettings));


nit:

Suggested change

envFailureMode: replacementFailureMode);

options.Converters.Add(new StringJsonConverterFactory(envOnlySettings));

envFailureMode: replacementFailureMode);

options.Converters.Add(new StringJsonConverterFactory(envOnlySettings));

RubenCerna2079 · 2025-10-07T23:20:59Z

src/Config/RuntimeConfigLoader.cs

+            using JsonDocument doc = JsonDocument.Parse(json);
+            if (doc.RootElement.TryGetProperty("azure-key-vault", out JsonElement akvElement))
+            {
+                return JsonSerializer.Deserialize<AzureKeyVaultOptions>(akvElement.GetRawText(), options);
+            }


Why do we want to deserialize the azure-key-vault here? Don't we already do that with the ConverterFactory?

souvikghosh04

added comments

souvikghosh04 · 2025-09-18T11:20:08Z

src/Config/RuntimeConfigLoader.cs

+        catch
+        {
+            // If we can't extract AKV options, return null and proceed without AKV variable replacement
+            return null;


I don't see anything explicitly handled in catch. It returns null as same outside the catch

souvikghosh04 · 2025-09-18T11:21:04Z

src/Config/RuntimeConfigLoader.cs

                    updatedConnection = GetConnectionStringWithApplicationName(connectionValue);
                }
-                else if (ds.DatabaseType is DatabaseType.PostgreSQL && replaceEnvVar)
+                else if (ds.DatabaseType is DatabaseType.PostgreSQL && replacementSettings?.DoReplaceEnvVar == true)


nit: replacementSettings?.DoReplaceEnvVar is true ?

souvikghosh04 · 2025-10-08T03:14:27Z

src/Config/DeserializationVariableReplacementSettings.cs

+                clientOptions.Retry.Mode = retryMode;
+                clientOptions.Retry.MaxRetries = options.RetryPolicy.MaxCount ?? 3;
+                clientOptions.Retry.Delay = TimeSpan.FromSeconds(options.RetryPolicy.DelaySeconds ?? 1);
+                clientOptions.Retry.MaxDelay = TimeSpan.FromSeconds(options.RetryPolicy.MaxDelaySeconds ?? 16);


should the MaxDelay be more than the NetworkTimeout? also are these configurable or going to be fixed always?

Aniruddh25

The changes look good to me. This is a good refactor. However, unit testing still remains and so does the test in AKV environment.

Aniruddh25 · 2025-10-30T01:34:04Z

src/Config/RuntimeConfigLoader.cs

+    /// <returns>AzureKeyVaultOptions if present, null otherwise.</returns>
+    private static AzureKeyVaultOptions? ExtractAzureKeyVaultOptions(string json, 
+        bool enableEnvReplacement,
+        Azure.DataApiBuilder.Config.Converters.EnvironmentVariableReplacementFailureMode replacementFailureMode = Azure.DataApiBuilder.Config.Converters.EnvironmentVariableReplacementFailureMode.Throw)


nit: do not need the prefix Azure.DataApiBuilder.Config since it is part of the same namespace.

Aniruddh25 · 2025-10-30T01:41:57Z

src/Service.Tests/Configuration/ConfigurationTests.cs

                runtimeConfig.ToJson(),
                out RuntimeConfig updatedRuntimeConfig,
-                replaceEnvVar: true);
+                new ());


nit: name the argument like

replacementSettings: new()

Aniruddh25 · 2025-10-30T01:45:01Z

src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs

                {
                    Assert.IsTrue(RuntimeConfigLoader.TryParseConfig(
-                        GetModifiedJsonString(repValues, @"""postgresql"""), out expectedConfig, replaceEnvVar: replaceEnvVar),
+                        GetModifiedJsonString(repValues, @"""postgresql"""), out expectedConfig, replacementSettings: new DeserializationVariableReplacementSettings(azureKeyVaultOptions: null, doReplaceEnvVar: replaceEnvVar, doReplaceAKVVar: true)),


We should add similar tests for AKV replacement checks

Aniruddh25 · 2025-10-30T01:49:30Z

src/Config/Converters/AzureKeyVaultOptionsConverterFactory.cs

+            throw new JsonException("Invalid AzureKeyVaultOptions format");
+        }
+
+        public override void Write(Utf8JsonWriter writer, AzureKeyVaultOptions value, JsonSerializerOptions options)


What changed in this file?

Aniruddh25 · 2025-10-31T00:41:47Z

src/Service.Tests/UnitTests/MySqlQueryExecutorUnitTests.cs

                        connectionString: connectionString,
-                        accessToken: CONFIG_TOKEN);
+                        accessToken: CONFIG_TOKEN,
+                        new());


please name the argument - what is this new() supplied to?

Aniruddh25 · 2025-10-31T00:42:20Z

src/Service.Tests/UnitTests/SqlQueryExecutorUnitTests.cs

                        connectionString: connectionString,
-                        accessToken: CONFIG_TOKEN);
+                        accessToken: CONFIG_TOKEN,
+                        new());


name the parameter, and apply the same suggestion every other point

Aniruddh25 · 2025-10-31T00:42:50Z

src/Service.Tests/UnitTests/PostgreSqlQueryExecutorUnitTests.cs

                        connectionString: connectionString,
-                        accessToken: CONFIG_TOKEN);
+                        accessToken: CONFIG_TOKEN,
+                        new());


name the argument

aaronburtle added 13 commits June 20, 2025 15:55

allow unused cache-control options without error

2fc665d

Merge branch 'main' of github.com:Azure/data-api-builder

50e54aa

Merge branch 'main' of github.com:Azure/data-api-builder

455ff18

Merge branch 'main' of github.com:Azure/data-api-builder

e0574bf

Merge branch 'main' of github.com:Azure/data-api-builder

033eeb0

Merge branch 'main' of github.com:Azure/data-api-builder

c7ad08f

...

049b23e

extend variable replacment and include AKV

cfdc2b5

change some logic for first pass var replacement

5eb9f0e

fix old bools being used

bbcc159

more bool replacements

f6f92bd

refactor out bools

0dd7ad6

Merge branch 'main' of github.com:Azure/data-api-builder

802eb53

RubenCerna2079 reviewed Oct 7, 2025

View reviewed changes

RubenCerna2079 requested changes Oct 7, 2025

View reviewed changes

souvikghosh04 requested changes Oct 8, 2025

View reviewed changes

aaronburtle added 3 commits October 9, 2025 13:29

Merge branch 'main' of github.com:Azure/data-api-builder

4e71c25

addressing comments

ac4356e

Merge branch 'main' of github.com:Azure/data-api-builder

c3dabe3

Aniruddh25 requested changes Oct 30, 2025

View reviewed changes

aaronburtle added 4 commits October 30, 2025 09:35

Merge branch 'main' of github.com:Azure/data-api-builder

6e77297

update names

3254af5

cleanup with fix

b28cd6b

cleanup

5e79339

Aniruddh25 reviewed Oct 31, 2025

View reviewed changes

	public bool DoReplaceAKVVar { get; set; } = true;
	public bool DoReplaceAkvVar { get; set; } = true;

		envFailureMode: replacementFailureMode);
		options.Converters.Add(new StringJsonConverterFactory(envOnlySettings));

Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882

Are you sure you want to change the base?

Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882

Conversation

aaronburtle commented Sep 18, 2025

Why make this change?

What is this change?

How was this tested?

Sample Request(s)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RubenCerna2079 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

souvikghosh04 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aniruddh25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants